-
Notifications
You must be signed in to change notification settings - Fork 55
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add 'obtain a storage key for non-storage purposes' #132
Conversation
This commit adds a new algorithm for obtaining a storage key that is suitable for use in non-storage APIs (for instance, BroadcastChannel). Specifically, this algorithm works regardless of whether storage access has been granted and also allows a storage key to be obtained for opaque origins.
storage.bs
Outdated
@@ -203,6 +203,16 @@ anticipated that some APIs will be applicable to both <a>storage types</a> going | |||
<p class=XXX>This is expected to change; see | |||
<a href="https://privacycg.github.io/storage-partitioning/">Client-Side Storage Partitioning</a>. | |||
|
|||
<p>To <dfn export>obtain a storage key for non-storage purposes</dfn>, given an <a>environment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note: I used the "run these steps" format for this instead of condensing it down into one line because the "run these steps" format will be helpful once we incorporate top-level site into this. If using the more concise version is preferred until then I'm happy to make the change in this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good to me. I'd maybe put it below the "main" algorithm, but I think this works.
storage.bs
Outdated
|
||
<ol> | ||
<li><p>Let <var>key</var> be <var>environment</var>'s | ||
<a for="environment settings object">origin</a>. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You forgot to remove this step from the algorithm below.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We still need the step there for the opaque origin check, but I'll change the variable name since 'key' there is no longer accurate
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see, we might need a bigger change.
In the future key will be at least (site, origin) + implementation-defined fields. Now if site is an opaque origin the algorithm below will need to return failure as well. So I think it does want to operate on the eventual key.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now if site is an opaque origin the algorithm below will need to return failure as well.
Off-topic for this PR, but I don't think we (Chrome) are currently planning to block storage in an embedded iframe merely because the top-level site has an opaque origin (i.e. as long as origin isn't opaque, the opaqueness of the site shouldn't matter).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the future key will be at least (site, origin) + implementation-defined fields. Now if site is an opaque origin the algorithm below will need to return failure as well. So I think it does want to operate on the eventual key.
Since the environment
object has the top-level-origin
field, it seems like determining whether the top-level origin is opaque might be easier (in either algorithm, as appropriate) using that field than using the site extracted from the storage key (IIUC).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we could just check if either key[0] or key[1] is an opaque origin, assuming we make it a tuple and those are the first two fields.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mkruisselbrink so you'd have some kind of ephemeral storage?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mkruisselbrink so you'd have some kind of ephemeral storage?
Yeah, behavior (and implementation) wise this would be pretty similar to storage in anonymous iframes (If I understand the anonymous iframe proposal correctly, the desire for anonymous iframes to be able to access (ephemeral) storage is one reason they don't want the iframes to have opaque origins).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The latest commit turns storage key into a tuple (with only one item for now) and uses its origin
for the opaque origin comparison. Per the tuple docs we can avoid using the indexing syntax if we name the tuple items, so I went with that approach.
Also, regarding whatwg/html#7567 (comment), I'll plan to add a storage key comparison function in a separate PR unless you all prefer that it be a part of this one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks great, thanks again!
Fixes #131
This commit adds a new algorithm for obtaining a storage key that is suitable for use in non-storage APIs (for instance, BroadcastChannel). Specifically, this algorithm works regardless of whether storage access has been granted and also allows a storage key to be obtained for opaque origins.
I don't think tests or implementation bugs are needed for this change since this PR maintains the existing functionality of "obtain a storage key" and doesn't use "obtain a storage key for non-storage purposes". If this is not the case let me know and I'm happy to write tests / open implementation bugs.
Preview | Diff